perf: Optimize substr_index to use bulk-NULL string builder#21877
perf: Optimize substr_index to use bulk-NULL string builder#21877martin-g merged 7 commits intoapache:mainfrom
substr_index to use bulk-NULL string builder#21877Conversation
…bstr-index # Conflicts: # datafusion/functions/src/unicode/substrindex.rs
| _ => builder.append_null(), | ||
| let num_rows = string_array.len(); | ||
| // Output is null IFF any input is null. | ||
| let nulls = NullBuffer::union( |
There was a problem hiding this comment.
Isn’t this highly dependent on heuristics—whether the cost of performing the bitmap union is actually justified by the append_nulls operations it helps us avoid?
Especially since the other approach of 1 by 1 scanning rows is also sequential IO ?
There was a problem hiding this comment.
Though I am not experienced enough to comment on the BitMap union performance of Arrow
There was a problem hiding this comment.
Good question, @RatulDawar !
Empirically, taking this approach appears to be a significant performance win (although in some cases the overhead of NULL handling might not be huge to begin with, depending on the UDF). Intuitively, it makes sense that this would be faster:
- Computing the union of two bitmaps can be accelerated with SIMD, and the Arrow primitives are well-optimized. It's also an entirely sequential memory access pattern.
append_valuein the bulk-NULL builder does not need to touch the NULL bitmap for every row, whereas in the Arrow builder it does. That means fewer per-row branches and also less data cache pressure.- Similarly, we can now just check a single bit to look for NULLs, rather than doing three conditionals. It's also easy for the compiler to hoist the is-the-NULL-bitmap-None check outside of the loop and elide it entirely (I haven't checked if LLVM is actually doing this but it could).
There was a problem hiding this comment.
- in the bulk-NULL builder does not need to touch the NULL bitmap for every row, whereas in the Arrow builder it does. That means fewer per-row branches and also less data cache pressure.
Makes sense, I can go ahead and draw a conclusion from this that in many cases, we can isolate simpler / coupled operations for better cache locality and SIMD.
Thanks for the explanation here @neilconway
|
Thank you, @neilconway , @Jefffrey & @RatulDawar ! |
4 participants
Which issue does this PR close?
substr_indexto use bulk-NULL string builder #21876.Rationale for this change
As with other recent optimizations, we can optimize NULL handling in
substr_indexby using the new bulk-NULL string builders.Benchmarks:
Utf8
Utf8View
This PR doesn't touch the Utf8View code path, so the Utf8View regressions above are likely measurement noise.
What changes are included in this PR?
substr_indexby switching from Arrow string builders to bulk-NULL string buildersAre these changes tested?
Yes, covered by existing tests.
Are there any user-facing changes?
No.